Remove `RETURNDATACOPY` elimination from UnusedStoreEliminator by rodiazet · Pull Request #16508 · argotorg/solidity

rodiazet · 2026-03-09T15:30:18Z

As discussed here we decided to remove this optimisation as it has a minimal impact on generated code and make the implementation of the UnusedStoreEliminator complicated and bug prone. In this case pattern returndatacopy(0, 0, returndatasize()) will never be eliminated.

cameel · 2026-03-09T19:33:11Z

Looks like removing this did not affect gas in any of our semantic tests?

Please check gas benchmarks from external tests though.

relative

ir-no-optimize

project bytecode_size deployment_gas method_gas

brink 0%

colony 0%

elementfi 0%

ens 0%

euler

gnosis

gp2 0%

pool-together 0%

uniswap 0% +0% +0%

yield_liquidator 0% +0% +0%

zeppelin

ir-optimize-evm+yul

project bytecode_size deployment_gas method_gas

brink 0%

colony 0%

elementfi 0%

ens 0% 0% 0%

euler 0% -0% +0%

gnosis

gp2 0%

pool-together 0%

uniswap 0% +0% -0%

yield_liquidator 0% -0% 0%

zeppelin 0% -0% +0%

ir-optimize-evm-only

project bytecode_size deployment_gas method_gas

brink 0%

colony 0%

elementfi 0%

ens 0% 0% 0%

euler

gnosis

gp2 0%

pool-together 0%

uniswap 0% 0% +0%

yield_liquidator 0% 0% 0%

zeppelin 0%

legacy-no-optimize

project bytecode_size deployment_gas method_gas

brink 0%

colony 0%

elementfi 0%

ens 0%

euler 0% +0% -0%

gnosis 0%

gp2 0%

pool-together 0%

uniswap 0% +0% +0%

yield_liquidator 0% -0% 0%

zeppelin

legacy-optimize-evm+yul

project bytecode_size deployment_gas method_gas

brink 0%

colony 0%

elementfi 0%

ens 0% 0% 0%

euler 0% -0% -0%

gnosis 0%

gp2 0%

pool-together 0%

uniswap 0% +0% -0%

yield_liquidator 0% -0% 0%

zeppelin 0% +0% +0%

legacy-optimize-evm-only

project bytecode_size deployment_gas method_gas

brink 0%

colony 0%

elementfi 0%

ens 0% 0% 0%

euler 0% +0% -0%

gnosis 0%

gp2 0%

pool-together 0%

uniswap 0% -0% +0%

yield_liquidator 0% 0% 0%

zeppelin

!V = version mismatch
!B = no value in the "before" version
!A = no value in the "after" version
!T = one or both values were not numeric and could not be compared
-0 = very small negative value rounded to zero
+0 = very small positive value rounded to zero

absolute

ir-no-optimize

project bytecode_size deployment_gas method_gas

brink 0

colony 0

elementfi 0

ens 0

euler

gnosis

gp2 0

pool-together 0

uniswap 0 36 82

yield_liquidator 0 12 72

zeppelin

ir-optimize-evm+yul

project bytecode_size deployment_gas method_gas

brink 0

colony 0

elementfi 0

ens 0 0 0

euler 0 -24 5972

gnosis

gp2 0

pool-together 0

uniswap 0 48 -103

yield_liquidator 0 -36 0

zeppelin 0 -24 121

ir-optimize-evm-only

project bytecode_size deployment_gas method_gas

brink 0

colony 0

elementfi 0

ens 0 0 0

euler

gnosis

gp2 0

pool-together 0

uniswap 0 0 3955

yield_liquidator 0 0 0

zeppelin 0

legacy-no-optimize

project bytecode_size deployment_gas method_gas

brink 0

colony 0

elementfi 0

ens 0

euler 0 12 -12110

gnosis 0

gp2 0

pool-together 0

uniswap 0 36 380

yield_liquidator 0 -12 0

zeppelin

legacy-optimize-evm+yul

project bytecode_size deployment_gas method_gas

brink 0

colony 0

elementfi 0

ens 0 0 0

euler 0 -48 -18878

gnosis 0

gp2 0

pool-together 0

uniswap 0 48 -1614

yield_liquidator 0 -12 0

zeppelin 0 100 1426

legacy-optimize-evm-only

project bytecode_size deployment_gas method_gas

brink 0

colony 0

elementfi 0

ens 0 0 0

euler 0 24 -10010

gnosis 0

gp2 0

pool-together 0

uniswap 0 -12 882

yield_liquidator 0 0 0

zeppelin

!V = version mismatch
!B = no value in the "before" version
!A = no value in the "after" version
!T = one or both values were not numeric and could not be compared
-0 = very small negative value rounded to zero
+0 = very small positive value rounded to zero

clonker

Looks good to me aside from the introduced error!

clonker

sorry for the nits

cameel

The main blocker for this is that it's missing regression tests for the bug. There are also some things to clarify about the buglist entry.

cameel · 2026-03-27T12:27:27Z

+        "link": "https://blog.soliditylang.org/2026/X/Y/Z/",
+        "introduced": "0.8.13",
+        "fixed": "0.8.35",
+        "severity": "low",


Did we already decide on severity for this?

I would not give it more than very low, but we also need to agree on that as a team.

I didn't know that there is something below low severity. Change it to very low. Can we just agreed here?

Severity levels are listed in the docs: List of Known Bugs

To agree on one we normally discuss it on a call or on the security channel.

cameel · 2026-03-27T14:07:49Z

 Bugfixes:
 * Yul: Fix incorrect serialization of Yul object names containing double quotes and escape sequences, producing output that could not be parsed as valid Yul.
 * Yul EVM Code Transform: Improve stack shuffler performance by fixing a BFS deduplication issue.
+* Yul Optimizer: Fix `UnusedStoreEliminator` incorrectly removing `returndatacopy` operations when the length comes from a stale `returndatasize()` call that was invalidated by subsequent call opcodes.


Entries for important bugs have their own section (that we only add when such bugfixes are present). Check how it was done for earlier releases.

Moved, but do we define this as important bug? How do we define them then?

If it gets a bug list entry it's an important bug.

We also always put this category at the top.

cameel

The main problem I still see here is that we don't have a repro for the bug. The included command-line test shows only that returndatacopy() in inline assembly was being removed, not that it could be removed wrongly due to stale returndatasize(). There's also no evmasm repro. The whole idea of this being an important bug in the first place hinges on it actually affecting users :)

Other than that, a few small nitpicks.

cameel · 2026-04-22T11:36:47Z

@@ -1,3 +1,4 @@
+// This optimisation step is removed and `returndatacopy` is never removed by `unusedStoreEliminator`.


"optimisation step" sounds like you're talking about the whole UnusedStoreEliminator. I'd explain it like this:

Suggested change

// This optimisation step is removed and `returndatacopy` is never removed by `unusedStoreEliminator`.

// Unused `returndatacopy()` is not supposed be optimized out in this case.

// In fact, it's never optimized out now that we removed this optimization from UnusedStoreEliminator.

It would fit the other test where you have this comment as well.

cameel · 2026-04-22T11:44:55Z

@@ -0,0 +1,12 @@
+// SPDX-License-Identifier: GPL-3.0
+pragma solidity >=0.8.0;


We don't hard-code versions in these.

Suggested change

pragma solidity >=0.8.0;

pragma solidity *;

Also, I'd group it with the other Yul optimizer tests: unusedStoreEliminator_returndatacopy_returndatasize/ -> yul_optimizer_unused_store_eliminator_unused_returndatasize_removal/

And the comment should also say that returndatasize() used to be removed but now the expectation is that now it is not. This is not obvious without context and the name does not make it clear either.

cameel · 2026-04-22T12:03:11Z

+        assembly {
+            // Destination offset 96 is chosen to make the written memory region [96, 96+returndatasize())
+            // provably disjoint from the ABI encoder's mload(64) (which reads [64, 96)).
+            returndatacopy(96, 0, returndatasize())


This verifies that the returndatacopy removal in inline assembly as possible and is no longer the case. Which is fine to have as a test, but not what I meant in #16508 (comment).

I expected something like the repro from the report. Which I guess would be this:

contract C { function f() public view { assembly { let s := returndatasize() pop(staticcall(gas(), 0x01, 0, 0, 0, 0)) returndatacopy(0, 0, s) } } }

I see that this does not reproduce the error though. Not even with the sequence we use in Yul tests for UnusedStoreEliminator (oxaSVj). Note also that this repro does not even compile - I had to remove the last argument from staticcall() to make it compile.

This puts into question if the bug can even be triggered. A buggy path definitely exists, but if there's no input that can exercise it, I would not classify it as an important bug.

Looks like the repro does work in pure Yul:

{ let s := returndatasize() pop(staticcall(gas(), 0x01, 0, 0, 0, 0)) returndatacopy(0, 0, s) }

So the bug is real after all. But the question still remains if it's reproducible in inline assembly. If it's not then I'd still consider demoting it.

We also still need a non-Yul version of the test to prove that affects evmasm as well (otherwise buglist entry needs viaIR: true).

BTW, I'd put the repro under yul_optimizer_unused_store_eliminator_stale_returndatasize_bug/ (Solidity version) and strict_asm_yul_optimizer_unused_store_eliminator_stale_returndatasize_bug/ (Yul version).

I added semantic test which triggers the bug.

Will move it to cmdLineTests.

Actually I think we should create the tests as you mentioned but the semantic test should stay too. It checks that the call reverts. cmdLineTest only verifies that the returndataload is not removed by the optimizer step.

cameel · 2026-04-22T12:14:13Z

+        "name": "UnusedStoreEliminatorStaleReturnDataSize",
+        "summary": "The Yul optimizer's ``UnusedStoreEliminator`` may incorrectly remove ``returndatacopy(...)`` operations when using a stale value from ``returndatasize()`` that was invalidated by subsequent call operations.",
+        "description": "The ``UnusedStoreEliminator`` is a Yul optimizer step that removes redundant memory and storage writes. One of the operations eligible for removal is ``returndatacopy(...)``. This particular operation has a quirk - unlike any other instruction for bulk memory copying it reverts on out-of-bounds access. A revert is one of the side-effects that the optimizer guarantees to preserve so the operation can only be removed when it is certain that it cannot revert. This is the case when the entire return data buffer is copied to memory, i.e. when the start offset is zero and the length equals ``returndatasize()``. The optimizer was special-cased to detect and optimize only this specific pattern, since it matches the code produced by the code generator for external calls. However, the check did not account for the possibility of ``returndatasize()`` values becoming stale. The size of the return data buffer is updated by ``call()``, ``staticcall()``, ``delegatecall()``, and ``callcode()``. If a ``returndatasize()`` value is stored in a variable before such an operation and then used in a subsequent ``returndatacopy(...)``, the stored size may no longer reflect the actual return data buffer size. Despite this, the optimizer would consider it safe to remove, bypassing the revert and allowing the code to continue, possibly leading to unexpected behavior. Since the code generator never produces code that interleaves multiple calls and access to their return data, the bug only affected inline assembly or handwritten Yul code. The necessary condition is the use of an optimizer sequence including the ``UnusedStoreEliminator`` step (which is the default).",
+        "link": "https://blog.soliditylang.org/2026/04/21/unusedstore-eliminator-stale-returndatasize-bug/",


The release is currently planned for the 29th.

Suggested change

"link": "https://blog.soliditylang.org/2026/04/21/unusedstore-eliminator-stale-returndatasize-bug/",

"link": "https://blog.soliditylang.org/2026/04/29/unusedstore-eliminator-stale-returndatasize-bug/",

This should be corrected in the blog post as well.

rodiazet mentioned this pull request Mar 9, 2026

Fix optimising out returndatacopy call. #16436

Closed

rodiazet requested review from cameel and nikola-matic and removed request for cameel March 9, 2026 16:11

cameel reviewed Mar 9, 2026

View reviewed changes

rodiazet force-pushed the remove-returndatacopy-elimination branch 2 times, most recently from 761080c to a3869e1 Compare March 10, 2026 18:37

clonker reviewed Mar 25, 2026

View reviewed changes

Comment thread docs/bugs.json Outdated

rodiazet force-pushed the remove-returndatacopy-elimination branch from a3869e1 to f817e10 Compare March 26, 2026 13:14

rodiazet requested review from cameel and clonker March 26, 2026 13:14

clonker reviewed Mar 26, 2026

View reviewed changes

Comment thread docs/bugs.json Outdated

rodiazet force-pushed the remove-returndatacopy-elimination branch from f817e10 to aca08f7 Compare March 27, 2026 11:31

rodiazet requested a review from clonker March 27, 2026 11:31

cameel requested changes Mar 27, 2026

View reviewed changes

cameel reviewed Mar 27, 2026

View reviewed changes

Comment thread docs/bugs.json Outdated

cameel mentioned this pull request Mar 27, 2026

Fix incorrect mstore removal. #16459

Open

cameel reviewed Mar 27, 2026

View reviewed changes

rodiazet force-pushed the remove-returndatacopy-elimination branch from aca08f7 to 9e623e9 Compare March 28, 2026 10:11

nikola-matic reviewed Mar 29, 2026

View reviewed changes

Comment thread libyul/optimiser/UnusedStoreEliminator.cpp Outdated

Comment thread libyul/optimiser/UnusedStoreEliminator.cpp Outdated

cameel mentioned this pull request Apr 2, 2026

Fix named error parameter encoding order in IR codegen #16453

Open

rodiazet force-pushed the remove-returndatacopy-elimination branch from 9e623e9 to fff3d3f Compare April 2, 2026 13:49

rodiazet requested review from cameel and nikola-matic April 2, 2026 13:51

rodiazet force-pushed the remove-returndatacopy-elimination branch 2 times, most recently from a2a9123 to 9d4c1d3 Compare April 17, 2026 11:50

cameel added this to the 0.8.35 milestone Apr 18, 2026

rodiazet mentioned this pull request Apr 21, 2026

Add blog post describing the returndatacopy bug and fix. argotorg/solidity-website#198

Open

rodiazet force-pushed the remove-returndatacopy-elimination branch from 9d4c1d3 to 8b6f9fb Compare April 21, 2026 11:47

rodiazet added the optimizer label Apr 21, 2026

rodiazet force-pushed the remove-returndatacopy-elimination branch from 8b6f9fb to b432744 Compare April 22, 2026 11:45

cameel reviewed Apr 22, 2026

View reviewed changes

rodiazet force-pushed the remove-returndatacopy-elimination branch 2 times, most recently from 18eafa4 to a15d018 Compare April 22, 2026 15:48

rodiazet added 5 commits April 22, 2026 17:52

Add returndataload elimination repro test.

4a250a1

Add evmasm test

45a5a29

Apply fix

0c9dcf1

Update tests

84a019d

Add changelog and bug list entry.

257d0fc

rodiazet force-pushed the remove-returndatacopy-elimination branch from a15d018 to 257d0fc Compare April 22, 2026 15:52

project	bytecode_size	deployment_gas	method_gas
brink	`0%`
colony	`0%`
elementfi	`0%`
ens	`0%`
euler
gnosis
gp2	`0%`
pool-together	`0%`
uniswap	`0%`	`+0%`	`+0%`
yield_liquidator	`0%`	`+0%`	`+0%`
zeppelin

		@@ -1,3 +1,4 @@
		// This optimisation step is removed and `returndatacopy` is never removed by `unusedStoreEliminator`.

	// This optimisation step is removed and `returndatacopy` is never removed by `unusedStoreEliminator`.
	// Unused `returndatacopy()` is not supposed be optimized out in this case.
	// In fact, it's never optimized out now that we removed this optimization from UnusedStoreEliminator.

		@@ -0,0 +1,12 @@
		// SPDX-License-Identifier: GPL-3.0
		pragma solidity >=0.8.0;

	"link": "https://blog.soliditylang.org/2026/04/21/unusedstore-eliminator-stale-returndatasize-bug/",
	"link": "https://blog.soliditylang.org/2026/04/29/unusedstore-eliminator-stale-returndatasize-bug/",

Conversation

rodiazet commented Mar 9, 2026

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rodiazet Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

relative

ir-no-optimize

ir-optimize-evm+yul

ir-optimize-evm-only

legacy-no-optimize

legacy-optimize-evm+yul

legacy-optimize-evm-only

absolute

ir-no-optimize

ir-optimize-evm+yul

ir-optimize-evm-only

legacy-no-optimize

legacy-optimize-evm+yul

legacy-optimize-evm-only

Uh oh!

clonker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

clonker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cameel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rodiazet Mar 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cameel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

rodiazet Mar 10, 2026 •

edited

Loading

`ir-no-optimize`

`ir-optimize-evm+yul`

`ir-optimize-evm-only`

`legacy-no-optimize`

`legacy-optimize-evm+yul`

`legacy-optimize-evm-only`

`ir-no-optimize`

`ir-optimize-evm+yul`

`ir-optimize-evm-only`

`legacy-no-optimize`

`legacy-optimize-evm+yul`

`legacy-optimize-evm-only`

rodiazet Mar 28, 2026 •

edited

Loading